Pserve --reload does not keep worker arguments. Fixes #2961 #2962
Conversation
|
r? @mmerickel |
|
Please don't mix whitespace/pep8 fixes in with the rest of the commit in the future. It makes it more difficult to see what the changes are. |
| @@ -1,9 +1,11 @@ | |||
| import mock | |||
There was a problem hiding this comment.
We don't currently use mock in the the Pyramid tests, instead we create dummy objects that have just the functionality required.
I don't think it is a great idea to add yet another dependency...
setup.py
Outdated
| @@ -65,9 +65,10 @@ | |||
| ] | |||
|
|
|||
| testing_extras = tests_require + [ | |||
| 'mock', | |||
There was a problem hiding this comment.
I'm a -1 on this, we use dummy objects everywhere else.
There was a problem hiding this comment.
By the way this dependencies are only installed when you run tests which is not something you do when you use the project.
There was a problem hiding this comment.
Sure, I understand that.
That's why I put them in another commit so that you can look at the diff without them.
It doesn't change anything to add another dependency, but I will try to rewrite my test with a Dummy object instead. (mock just makes it easy to do so with less code to maintain.) |
There was a problem hiding this comment.
This looks correct but it does need to stop using mock as @bertjwregeer mentioned.
|
ping @Natim |
|
Hi @mmerickel, I didn't forget about this and I am going to rewrite tests without mocks. |
|
@Natim Okie sounds good.. just wanted to make sure it wasn't forgotten! |
|
To be honest I find the code much harder to maintain with a test like that. Are you sure it worth getting rid of the mock dependency? |
Natim
force-pushed
the
2961-hupper-call-broken
branch
2 times, most recently
from
432db17 to
4f1e551
Compare
Natim
force-pushed
the
2961-hupper-call-broken
branch
from
4f1e551 to
1702daa
Compare
|
@bertjwregeer @mmerickel how does it look to you now? |
|
Any news about it? |
| start_reloader=dummy_start_reloader) | ||
|
|
||
| inst = self._makeOne('--reload', 'development.ini') | ||
| inst.run() |
There was a problem hiding this comment.
I know we aren't using mock (sorry!) but if you're patching a module-scope value, you need to replace it after the test is over. For example:
orig_hupper = pserve.hupper
try:
pserve.hupper = ...
inst = ...
inst.run()
finally:
pserve.hupper = orig_hupper
Why cannot we start? |
Well I'm not adding a new dependency in a bugfix release and I imagine we want to backport this to 1.8. I'm definitely open to improving this situation in 1.9 though. |
Ok it makes sense 👍 |
|
Just waiting on @bertjwregeer to approve this before I continue. @Natim if you come back with more bugs the day after I cut a new release again I'm going to bonk you on the head if I ever see you in person! |
|
I will say that I am not all that excited about using Mock. I feel like mock is overused for "magic" to make tests pass, and it just overall makes a bad experience for writing tests. I think we can try to use it, but we have to be careful to not be overly reliant on it. |
|
Hopefully releases are cheap :) |
|
@Natim I wish that were true. :-( |
Refs #2961